fix(engine): key PostgreSQL progress to every apply, not one slot - #1130

Pull request overview

This PR fixes PostgreSQL engine progress tracking so concurrent applies on the same long-lived engine instance don’t overwrite each other’s in-memory progress, preventing a running apply from incorrectly observing the idle sentinel (“No active schema change”) when another apply is accepted.

Changes:

• Replace the single progress “slot” with a map keyed by apply identity (ResumeState.MigrationContext) so each apply reads its own progress.
• Retire already-terminal progress entries on acceptance of a new apply to bound memory usage without evicting in-flight entries.
• Expand unit tests to cover concurrent applies, terminal-entry retirement, discard behavior for untracked writes, and drain semantics.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

🤖 Reviewed on Morgan's behalf by his AI agent. Not approving yet — one question below. Everything else checks out, and this is a clear improvement over the single slot.

What I verified:

• The map is bounded. claimProgress prunes terminal entries on every accept, so the steady state is {running} ∪ {just-claimed}; terminal entries only accumulate between accepts and the next accept sweeps them. No leak.
• No resurrection after retirement. A late publishProgress for a pruned key hits the !tracked guard and is discarded rather than re-inserting a dead entry.
• Drain → nil map is safe. e.progress[key] on a nil map returns nil, so Progress takes the idle-sentinel branch, and claimProgress re-makes the map. No nil-map write.
• The idle sentinel string is untouched, so the verbatim comparison stale-task recovery depends on still matches.

The question — terminal retirement across a lease handover. The retirement rule's justification is "a driver that has accepted another apply on this target has moved past it." That's a statement about one driver's sequencing, but the scenario this PR fixes is explicitly a second driver re-driving work after a lease handover while the first goroutine is still executing.

So: driver A's apply reaches terminal. Driver B accepts a new apply on the same target, which retires A's entry. A polls and gets No active schema change instead of A's actual terminal outcome. Running entries are protected from this; terminal ones are not.

The PR body's answer is that such a poller "settles against the target schema, which is authoritative" — and I think that's probably right, since a terminal outcome is recoverable from the schema in a way that a vanished running apply is not. That asymmetry is what makes the trade sound. But I couldn't confirm from this diff that every path polling a terminal PostgreSQL apply actually reaches that settle-from-schema fallback, rather than surfacing the idle sentinel as "your apply disappeared" — which would be the old bug wearing a different hat.

If you can point at the recovery path that absorbs it (or confirm a retired terminal entry is only ever read by a driver that has already recorded the outcome), I'm happy to stamp. Alternatively, keeping terminal entries until Drain would sidestep the question entirely, at the cost of the bound above — and given the map is small and Drain runs on every recovery/re-plan, that cost looks close to nothing.

Style note, non-blocking: the doc comments here are genuinely good — they explain why the slot changed hands rather than restating the code. Worth keeping that bar.

🤖 Approved on Morgan's behalf by his AI agent. Resolving my own hold — I asked whether a poller reading the idle sentinel for a retired terminal entry actually reaches the settle-from-schema fallback, and said I couldn't confirm it from the diff. I've now traced it in source. Your answer in the PR body was right, and the mechanism is stronger than the description claims.

The path, end to end:

1. Progress returns {StatePending, "No active schema change"} for the retired key.
2. engineReportsLostWork fires, and lostEngineWorkPendingBudget returns 0 for this engine precisely because it declares RegistersWorkSynchronously — so observePending reports exhausted on the first report, with no window in which the driver sits on a stale answer.
3. settleLostEngineWork then reads the target directly, and its contract is exactly the right one:

A target that already has the desired schema means the work finished and only its outcome was lost: the task completes. A target that still needs the change means the work is genuinely gone: the task is marked retryable so a fresh claim re-drives it — never permanently failed, because nothing about the target is known to be broken.

So the race I was worried about — apply A completes, driver B's accept retires A's terminal entry, A polls for the first time afterwards — lands on schema verification, sees the change present, and completes the task correctly. Not lost, not failed. The retirement is safe.

Worth noting the nice interaction: declaring SynchronousWorkRegistration is what makes this good rather than merely survivable. Without it the driver would spend the full trust budget sitting on an idle sentinel before verifying; with it, verification is immediate. The capability and the retirement rule reinforce each other.

I also checked the one carve-out in settleLostEngineWork — a revert-phase task can't be settled by reading the target, since the forward change already cut over and a schema match proves nothing. That would be a genuine hole in this reasoning, except PostgreSQL doesn't support revert or skip-revert at all (per the envelope doc in #1144), so it isn't reachable for this engine. Worth keeping in mind if PostgreSQL ever grows revert.

Everything from my first review stands: the map is bounded by the prune-on-accept, publishProgress's !tracked guard prevents resurrection, the nil-map read in Progress is safe, and the sentinel string is untouched so the cross-engine contract holds.